Fix missing original env variable issue on multiple test cases. #3190

john-yan · 2015-10-05T21:16:35Z

When the parent spawn the child processes, the environment variables passing into the child processes are missing the original env variables passing into the parent. Some missing variables like LD_LIBRARY_PATH cause the child processes unable to run.

PR-URL: #3183

rvagg · 2015-10-05T22:10:36Z

copied my comments from #3191

lgtm

I imagine the proper way to do this is with Object.assign() but since it's tests I'm not sure it matters, anyone else have an opinion on that?

mhdawson · 2015-10-06T19:25:54Z

I think @cjihrig was suggesting using this approach

env: util._extend(process.env, {NODE_DEBUG: process.argv[2]})

john-yan · 2015-10-06T19:57:07Z

@rvagg @cjihrig @mhdawson Hello, I don't mind to change to Object.assign, extend or something else, because it shouldn't make any difference from the testing perspective. But which one do you agree on?

cjihrig · 2015-10-06T20:11:32Z

I forgot we have Object.assign() now. If you can use that, that's probably the correct approach.

john-yan · 2015-10-06T23:54:50Z

@rvagg @cjihrig Redo commit with Object.assign()

cjihrig · 2015-10-07T00:24:56Z

test/parallel/test-fs-readfile-error.js

@@ -16,7 +16,7 @@ var callbacks = 0;
 function test(env, cb) {
  var filename = path.join(common.fixturesDir, 'test-fs-readfile-error.js');
  var execPath = '"' + process.execPath + '" "' + filename + '"';
-  var options = { env: env || {} };
+  var options = { env: Object.assign(process.env, env) || {} };


I think you can drop the || {} now.

cjihrig · 2015-10-07T00:28:30Z

test/sequential/test-util-debug.js

+      // env variable is passed to the child to make the test pass.
+      // this is fixed in the next version of lttng (2.7+), so we can
+      // remove it at sometime in the future.
+      HOME: process.env.HOME


Is HOME still required? Seems like it should get copied over now.

Yeah, thanks, you are right. Home should be copied over. Fixed.

cjihrig · 2015-10-07T00:41:13Z

LGTM

mhdawson · 2015-10-07T20:30:58Z

lgtm will land tomorrow

jasnell · 2015-10-08T04:25:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/449/

jasnell · 2015-10-08T04:51:07Z

Seeing a CI failure on freebsd (https://ci.nodejs.org/job/node-test-commit-other/854/nodes=freebsd101-64/console) that may be related to this commit.

not ok 886 test-child-process-emfile.js
#
#assert.js:89
#  throw new assert.AssertionError({
#  ^
#AssertionError: 0 undefined null
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

@john-yan @mhdawson ... can you please investigate before landing in master. If landed in master in time, I'll pick it back to v4.x but not sure it'll make it for v4.2.0

evanlucas · 2015-10-08T10:59:33Z

I've been seeing that test fail more often

jbergstroem · 2015-10-08T11:21:20Z

Pretty sure that test isn't related: #3193

LinusU · 2015-10-08T13:38:55Z

Object.assign actually modifies the first argument, should we really send process.env there?

Maybe do this instead:

- Object.assign(process.env, { foo: expected })
+ Object.assign({}, process.env, { foo: expected })

john-yan · 2015-10-08T14:23:35Z

Hello @LinusU , looks like they wanted to extend process.env instead.

john-yan · 2015-10-08T14:34:27Z

Hello @jasnell , Given that the changes in this commit has no global effect and no change has been made to the failing test cases in the CI, I don't think it's related.

jasnell · 2015-10-08T14:45:39Z

Ok. Thank you for clarifying. Running one more local test then will land on master.

Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #3190

jasnell · 2015-10-08T14:54:52Z

Landed in a9d42e0

Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #3190

rvagg mentioned this pull request Oct 5, 2015

Fix missing original env variable issue on multiple test cases. #3191

Closed

mscdex added the test Issues and PRs related to the tests. label Oct 6, 2015

mhdawson self-assigned this Oct 6, 2015

mhdawson added the land-on-v4.x label Oct 6, 2015

john-yan force-pushed the master branch 2 times, most recently from 8a16df9 to 7e8844d Compare October 6, 2015 23:52

john-yan force-pushed the master branch from 7e8844d to fb10cb4 Compare October 6, 2015 23:55

cjihrig reviewed Oct 7, 2015
View reviewed changes

john-yan force-pushed the master branch from fb10cb4 to a468875 Compare October 7, 2015 00:26

cjihrig reviewed Oct 7, 2015
View reviewed changes

test: fix losing original env vars issue

2dfcce0

john-yan force-pushed the master branch from a468875 to 2dfcce0 Compare October 7, 2015 00:31

jasnell pushed a commit that referenced this pull request Oct 8, 2015

test: fix losing original env vars issue

a9d42e0

Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #3190

jasnell closed this Oct 8, 2015

jasnell pushed a commit that referenced this pull request Oct 8, 2015

test: fix losing original env vars issue

26a7ec6

Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> PR-URL: #3190

jasnell mentioned this pull request Oct 8, 2015

Release proposal: v4.2.0 'Argon' (LTS) #3258

Closed

jasnell added the lts-landed-on-v4.x label Oct 8, 2015

MylesBorins removed the land-on-v4.x label Nov 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing original env variable issue on multiple test cases. #3190

Fix missing original env variable issue on multiple test cases. #3190

john-yan commented Oct 5, 2015

rvagg commented Oct 5, 2015

mhdawson commented Oct 6, 2015

john-yan commented Oct 6, 2015

cjihrig commented Oct 6, 2015

john-yan commented Oct 6, 2015

cjihrig Oct 7, 2015

john-yan Oct 7, 2015

cjihrig Oct 7, 2015

john-yan Oct 7, 2015

cjihrig commented Oct 7, 2015

mhdawson commented Oct 7, 2015

jasnell commented Oct 8, 2015

jasnell commented Oct 8, 2015

evanlucas commented Oct 8, 2015

jbergstroem commented Oct 8, 2015

LinusU commented Oct 8, 2015

john-yan commented Oct 8, 2015

john-yan commented Oct 8, 2015

jasnell commented Oct 8, 2015

jasnell commented Oct 8, 2015

Fix missing original env variable issue on multiple test cases. #3190

Fix missing original env variable issue on multiple test cases. #3190

Conversation

john-yan commented Oct 5, 2015

rvagg commented Oct 5, 2015

mhdawson commented Oct 6, 2015

john-yan commented Oct 6, 2015

cjihrig commented Oct 6, 2015

john-yan commented Oct 6, 2015

cjihrig Oct 7, 2015

Choose a reason for hiding this comment

john-yan Oct 7, 2015

Choose a reason for hiding this comment

cjihrig Oct 7, 2015

Choose a reason for hiding this comment

john-yan Oct 7, 2015

Choose a reason for hiding this comment

cjihrig commented Oct 7, 2015

mhdawson commented Oct 7, 2015

jasnell commented Oct 8, 2015

jasnell commented Oct 8, 2015

evanlucas commented Oct 8, 2015

jbergstroem commented Oct 8, 2015

LinusU commented Oct 8, 2015

john-yan commented Oct 8, 2015

john-yan commented Oct 8, 2015

jasnell commented Oct 8, 2015

jasnell commented Oct 8, 2015